Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document that using assert() is frowned upon #1807

Conversation

DemiMarie
Copy link
Contributor

Graceful error handling is preferred.

Graceful error handling is preferred.
@DemiMarie DemiMarie changed the title Enhance CONTRIBUTING.md Document that using assert() is frowned upon Oct 21, 2021
@pmatilai
Copy link
Member

Sorry but no, this misses the point by a mile.

Most of rpm is a shared library. Such a thing cannot be blowing up every which way in random spots.

I refuse to start documenting common sense.

@pmatilai pmatilai closed this Oct 25, 2021
@cgwalters
Copy link
Contributor

I think this is a nuanced topic. Very recently, I started regretting an assertion we added in rpm-ostree (not a library, but a daemon providing a service, so acts like a library in some ways). It's tangentially related to rpm actually, over here: coreos/rpm-ostree#3183 around dealing with the bdb/sqlite transition.
To remove that assertion I need to go through and change various internal APIs, which is definitely the right thing here.

But then in ostree more recently, I started adding more assertions to help out gcc -fanalyzer. I tried building rpm with -fanalyzer and it looks like there might be some similar needs (or potentially real bugs).

See also this article about how missing these types of assertions can lead to the gcc optimizer doing something you really don't want.

@pmatilai
Copy link
Member

Yup. These kind of reasons are why I don't want any "assert is evil" written down in our guidelines: it's a tool like any other and has it's uses, but public API input checking is not one of those.

@DemiMarie DemiMarie deleted the coding-style-no-asserts branch February 7, 2022 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants